Should I wait for the last todos to be done?

Should I wait for the last todos to be done?

Ah @domoritz I wasn't expecting you so soon!

Yeah I'm done now, was just filling out the description to make reviewing easier.

• feat: Improve flights.* dataset reproducibility #645 (comment)
  • This one I wanted to highlight that flights-1k.csv is just for pre-merge demo purposes
• feat: Improve flights.* dataset reproducibility #645 (comment)
  • This is just to provide context during the review

Edit

You should be able to simply run the script locally, without arguments.
If we wanted to run this in CI, I'd probably need to add a static seed for polars.DataFrame.sample

Great. @dsmedia can you do a detailed review? I will do a rough check. We don't need to run the scripts on the CI.

#645 (comment)

Thanks @domoritz

This looks great @dangotbanned. The pattern of downloading data from this API, transforming it, and generating clean sample datasets of various sizes is something that I could see being useful for analysis of this flights dataset well beyond the purposes of this repo.

Given that broader potential, one suggestion that might add some flexibility to the valid row counts while keeping things clean: Instead of restricting row counts to specific numbers,

type Rows = Literal[
    1_000,
    2_000,
    5_000,
    10_000,
    20_000,
    100_000,
    200_000,
    500_000,
    1_000_000,
    3_000_000,
    5_000_000,
    10_000_000,
    100_000_000,
    500_000_000,
    1_000_000_000,
]
"""Number of rows to include in the output."""

we could allow any multiple of 1,000 for thousands and any multiple of 1,000,000 for millions. For example:

def validate_row_count(n: int) -> bool:
    if n >= 1_000_000:
        return n % 1_000_000 == 0  # Must be exact millions
    return n % 1_000 == 0  # Must be exact thousands

This would let users choose numbers like:

• 66,000 -> "flights-66k.parquet"
• 123,000 -> "flights-123k.parquet"
• 676,000,000 -> "flights-676m.parquet"

The nice thing about this approach is that:

1. It maintains unambiguous filenames (each valid number maps to exactly one filename)
2. Filenames stay clean and readable (no decimals)
3. Gives users more flexibility in choosing sample sizes for different use cases (e.g., testing, benchmarking, demos)
4. The validation rule is simple to understand - just use exact thousands/millions

If this script gets adopted by other projects (which I could definitely see happening), this flexibility would be particularly valuable. Different projects might need different sample sizes - some might want 66k rows for a specific benchmark, others might need 676m rows to test large-scale processing. Having this flexibility while maintaining clean, predictable filenames would make the script more broadly useful.

#645 (comment)

Thanks for taking a look @dsmedia!

Given that broader potential, one suggestion that might add some flexibility to the valid row counts while keeping things clean: Instead of restricting row counts to specific numbers,

I didn't do a job documenting this well here.
The fixed values defined in Rows lean more towards hints; than a validation rule.
The actual logic in Spec.name is really just checking n_rows >= 1_000:

So the 3 examples you gave all work currently (at runtime), but 999 fails:

from scripts.flights2 import Spec, DateRange

date_range = DateRange((2001, 1), (2001, 12))
spec_0 = Spec(date_range, 66_000, ".parquet")
spec_1 = Spec(date_range, 123_000, ".parquet")
spec_2 = Spec(date_range, 676_000_000, ".parquet")
spec_bad = Spec(date_range, 999, ".parquet")

print(spec_0.name)
print(spec_1.name)
print(spec_2.name)
print(spec_bad.name)

flights-66k.parquet
flights-123k.parquet
flights-676m.parquet

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[4], line 12
     10 print(spec_1.name)
     11 print(spec_2.name)
---> 12 print(spec_bad.name)

File ../vega-datasets/scripts/flights2.py:415, in Spec.name(self)
    413     s = f"{frac}k"
    414 else:
--> 415     raise TypeError(self.n_rows)
    416 return f"{self._name_prefix}{s}{self.suffix}"

TypeError: 999

Admittedly though, all 4 of these equally trigger a type checker warning (statically) - which is not very intuitive:

The resolved environment for this script has a transient dependency which has some examples of runtime Annotated validation:

https://github.com/annotated-types/annotated-types/blob/b60ad7bff90019c7de3547df1c8e3586eb328423/annotated_types/__init__.py#L204-L225

I explored this a little with the DateTimeFormat type.
There, I've used a very loose approximation of what would be accepted by chrono

Question

@dsmedia would you be happy if I rewrite n_rows: Rows more in the style of dt_format: DateTimeFormat?

This would look like:

• Some more content in the docstring
• Replacing Literal[...] -> Annotated[int, is_rows]
• Adding a runtime check for Spec.n_rows like Spec.dt_format
  • With the TypeError message explaining the constraint

Updated

@dsmedia hopefully I've addressed your concerns in (8ec3adf)

Since the validation logic started getting a bit unwieldy in Spec.__init__, I've moved that to Spec._validate.
There's also checks for valid columns and file extension in there as well